Remove checks on conditions in the 'ms_must_reindex?' method #429
Conversation
This commit removes the checks on the meilisearch conditions options `:if` and `:unless` in the `ms_must_reindex?` method as they are incorrectly used. ## Low level reason [Indeed the README documents an example on how to avoid reindexing when an `if:` condition is defined (e.g. `if: :published?`)](https://github.com/meilisearch/meilisearch-rails/tree/main?tab=readme-ov-file#conditional-indexing). However the documented way doesn't work: Indeed, one would need to define a method `#will_save_change_to_published??` (note the two question marks) to be able to prevent reindexing. However ruby doesn't allow such a method name (with double trailing question marks). ## Higher level reason In a high level vision, the `if:` and `unless:` conditions are used as “constraints” when the lib will try to index a resource in many different places of the code by executing `Utilities.indexable?(...)` before trying to index, those those conditions are sufficient to “enable” or “disable” the indexation of the lib, no matter what attributes change on the record. Why would the “must_reindex” logic try to check those conditions? I admit I didn't understand and believe this code is obsolete. What do you folks think?
WalkthroughThe Changes
Sequence DiagramsequenceDiagram
participant Caller
participant ms_must_reindex
rect rgb(240, 248, 255)
Note over Caller,ms_must_reindex: Before: With Conditional Logic
Caller->>ms_must_reindex: Call with options {:if, :unless}
ms_must_reindex->>ms_must_reindex: Evaluate :if option
ms_must_reindex->>ms_must_reindex: Evaluate :unless option
ms_must_reindex-->>Caller: Return boolean (conditional-based)
end
rect rgb(248, 240, 255)
Note over Caller,ms_must_reindex: After: Simplified Logic
Caller->>ms_must_reindex: Call
ms_must_reindex->>ms_must_reindex: Check primary key changes
ms_must_reindex->>ms_must_reindex: Check attribute changes
ms_must_reindex-->>Caller: Return boolean (changes-based only)
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10–15 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This is not true. The previous code didn't “evaluate” the |
1 participant
This commit removes the checks on the meilisearch conditions options
:ifand:unlessin thems_must_reindex?method as they are incorrectly used.Low level reason
Indeed the README documents an example on how to avoid reindexing when an
if:condition is defined (e.g.if: :published?). However the documented way doesn't work: Indeed, one would need to define a method#will_save_change_to_published??(note the two question marks) to be able to prevent reindexing. However ruby doesn't allow such a method name (with double trailing question marks).Higher level reason
In a high level vision, the
if:andunless:conditions are used as “constraints” when the lib will try to index a resource in many different places of the code by executingUtilities.indexable?(...)before trying to index, those those conditions are sufficient to “enable” or “disable” the indexation of the lib, no matter what attributes change on the record.Why would the “must_reindex” logic try to check those conditions? I admit I didn't understand and believe this code is obsolete.
What do you folks think?
Pull Request
Related issue
Fixes #<issue_number>
What does this PR do?
PR checklist
Please check if your PR fulfills the following requirements:
Thank you so much for contributing to Meilisearch!
Summary by CodeRabbit
:ifand:unless), eliminating complex conditional evaluation logic.✏️ Tip: You can customize this high-level summary in your review settings.